Vibe mcp apps #6569
Vibe mcp apps #6569
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces an apps platform extension that enables users to create custom HTML/CSS/JavaScript applications through chat interactions with goose. The extension leverages LLM-based code generation to build standalone apps that run in sandboxed windows.
Changes:
- Adds a new "apps" platform extension with tools for creating, iterating, deleting, and listing custom apps
- Implements platform event notifications to synchronize app lifecycle (create/update/delete) between backend and frontend
- Adds import/export functionality for sharing apps as HTML files with embedded metadata
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/desktop/src/utils/platform_events.ts | New module for handling platform event notifications from backend via MCP streaming |
| ui/desktop/src/utils/conversionUtils.ts | Added formatAppName utility to convert kebab-case/snake_case names to Title Case |
| ui/desktop/src/preload.ts | Added IPC methods for refreshApp and closeApp window operations |
| ui/desktop/src/main.ts | Implements app window tracking and handlers for refresh/close operations |
| ui/desktop/src/hooks/useChatStream.ts | Integrates platform event handling into chat stream notifications |
| ui/desktop/src/components/apps/StandaloneAppView.tsx | Uses formatAppName for document title |
| ui/desktop/src/components/apps/AppsView.tsx | Major update: adds import/export UI, platform event listener, and improved empty states |
| ui/desktop/src/components/GooseSidebar/AppSidebar.tsx | Changes apps visibility from checking for existing apps to checking if apps extension is enabled |
| ui/desktop/src/App.tsx | Registers global platform event handlers in AppInner component |
| ui/desktop/src/api/* | Generated API types and SDK for new export_app and import_app endpoints |
| crates/goose/src/goose_apps/mod.rs | Adds GooseApp HTML serialization/deserialization with embedded JSON-LD metadata and PRD support |
| crates/goose/src/conversation/message.rs | Extends SystemNotificationContent with optional data field for platform events |
| crates/goose/src/agents/apps_extension.rs | New 750-line extension implementing apps management with LLM-based code generation |
| crates/goose/src/agents/extension.rs | Adds result_with_platform_notification helper for attaching notifications to tool results |
| crates/goose/src/agents/agent.rs | Handles platform notifications from tool result metadata and emits them as MCP notifications |
| crates/goose-server/src/routes/agent.rs | Adds export_app and import_app HTTP endpoints |
| and that will appear here. Or if somebody shared an, app you can import it using | ||
| the button above.i |
There was a problem hiding this comment.
Typo in empty state message. "an, app" should be "an app" - extra comma and space.
| and that will appear here. Or if somebody shared an, app you can import it using | |
| the button above.i | |
| and that will appear here. Or if somebody shared an app you can import it using | |
| the button above. |
| and that will appear here. Or if somebody shared an, app you can import it using | ||
| the button above.i |
There was a problem hiding this comment.
Incomplete sentence. The sentence ends with "the button above.i" where the ".i" should likely be removed or completed.
| and that will appear here. Or if somebody shared an, app you can import it using | |
| the button above.i | |
| and that will appear here. Or if somebody shared an app, you can import it using | |
| the button above. |
crates/goose/src/goose_apps/mod.rs
Outdated
| let mcp_resource = McpAppResource { | ||
| uri: resource.uri.clone(), | ||
| name: format_resource_name(resource.name.clone()), | ||
| name: resource.name.clone(), |
There was a problem hiding this comment.
The name formatting was removed from the MCP resource name, which changes the behavior for apps from MCP servers. This could cause display inconsistencies if MCP servers provide names with underscores or no capitalization, since the frontend now formats these names for display using formatAppName(). Consider whether the removal is intentional or if name formatting should be preserved for consistency.
| console.error('Failed to export app:', err); | ||
| setError(err instanceof Error ? err.message : 'Failed to export app'); |
There was a problem hiding this comment.
The error messages here don't provide context about which app failed to export. Consider including the app name in the error message to help with debugging.
| console.error('Failed to export app:', err); | |
| setError(err instanceof Error ? err.message : 'Failed to export app'); | |
| console.error(`Failed to export app "${formatAppName(app.name)}":`, err); | |
| setError( | |
| err instanceof Error | |
| ? err.message | |
| : `Failed to export app "${formatAppName(app.name)}"` | |
| ); |
| let metadata: serde_json::Value = serde_json::from_str(json_str) | ||
| .map_err(|e| format!("Failed to parse metadata JSON: {}", e))?; | ||
|
|
||
| let name = metadata | ||
| .get("name") | ||
| .and_then(|v| v.as_str()) | ||
| .ok_or("Missing 'name' in metadata")? | ||
| .to_string(); |
There was a problem hiding this comment.
The metadata validation doesn't check for the correct "@context" and "@type" values. The code should validate that the metadata has "@context": "https://goose.ai/schema" and "@type": "GooseApp" to ensure it's actually a Goose app and not arbitrary JSON-LD.
| if (targetApp) { | ||
| await window.electron.launchApp(targetApp).catch((err) => { | ||
| console.error('Failed to launch newly created app:', err); | ||
| }); |
There was a problem hiding this comment.
When app_created fires but targetApp is not found, the event is silently ignored. This could happen if there's a timing issue or the app wasn't properly cached. Consider logging a warning when app_created event occurs but no app is found, to help debug issues.
| }); | |
| }); | |
| } else { | |
| console.warn( | |
| `[platform_events] app_created event received but no matching app found`, | |
| { app_name, sessionId, fetchedAppNames: apps.map((a: GooseApp) => a.name) } | |
| ); |
| let mut app = GooseApp::from_html(&body.html).map_err(|e| ErrorResponse { | ||
| message: format!("Invalid Goose App HTML: {}", e), | ||
| status: StatusCode::BAD_REQUEST, | ||
| })?; | ||
|
|
||
| let original_name = app.resource.name.clone(); | ||
| let mut counter = 1; | ||
|
|
||
| let existing_apps = cache.list_apps().unwrap_or_default(); | ||
| let existing_names: HashSet<String> = existing_apps | ||
| .iter() | ||
| .map(|a| a.resource.name.clone()) | ||
| .collect(); | ||
|
|
||
| while existing_names.contains(&app.resource.name) { | ||
| app.resource.name = format!("{}_{}", original_name, counter); | ||
| app.resource.uri = format!("ui://apps/{}", app.resource.name); | ||
| counter += 1; | ||
| } | ||
|
|
||
| cache.store_app(&app).map_err(|e| ErrorResponse { | ||
| message: format!("Failed to store app: {}", e), | ||
| status: StatusCode::INTERNAL_SERVER_ERROR, | ||
| })?; |
There was a problem hiding this comment.
The import_app function doesn't set the mcp_server field when importing an app. The imported app should have mcp_server set to "apps" so it's properly associated with the apps extension, otherwise it may not appear correctly in the UI or be handled properly by the cache.
| "name": "Clock", | ||
| "description": "Swiss Railway Clock", | ||
| "width": 300, | ||
| "height": 300, | ||
| "resizable": false |
There was a problem hiding this comment.
The metadata in the clock.html example uses "Clock" as the name, but the naming convention in other parts of the code (like formatAppName) expects lowercase-with-hyphens format. The clock should use "clock" as the name for consistency, since the system converts names for display formatting.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| <p className="text-sm text-text-muted"> | ||
| Install MCP servers that provide UI resources to see apps here. | ||
| Open a chat and ask goose for the app you want to have. It can build one for you | ||
| and that will appear here. Or if somebody shared an app, you can import it using |
There was a problem hiding this comment.
Typo in help text: "an, app" should be "an app".
| Install MCP servers that provide UI resources to see apps here. | ||
| Open a chat and ask goose for the app you want to have. It can build one for you | ||
| and that will appear here. Or if somebody shared an app, you can import it using | ||
| the button above. |
There was a problem hiding this comment.
Incomplete sentence - ends with "above.i" instead of proper punctuation. Should be "above." (remove the trailing 'i').
| app.resource.uri = format!("ui://apps/{}", app.resource.name); | ||
| counter += 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
The import functionality has a potential issue: name conflicts are resolved by appending "_1", "_2", etc., but the mcpServer field from the imported HTML is preserved. This could be misleading - an imported app might claim to be from extension "foo" but actually be from a different source. Consider either clearing the mcpServer field on import or validating it against active extensions.
| // Clear any imported MCP server binding to avoid spoofing the app's origin. | |
| if app.resource.mcp_server.is_some() { | |
| app.resource.mcp_server = None; | |
| } |
| let mut app = GooseApp::from_html(&body.html).map_err(|e| ErrorResponse { | ||
| message: format!("Invalid Goose App HTML: {}", e), | ||
| status: StatusCode::BAD_REQUEST, | ||
| })?; |
There was a problem hiding this comment.
App names extracted from imported HTML are used directly in file paths without validation. Malicious app names containing path traversal sequences (like "../", "..\") or special characters could potentially access files outside the apps directory. Consider validating that app names only contain safe characters (alphanumeric, hyphens, underscores) and don't contain path separators.
| goosedClients.set(appWindow.id, launchingClient); | ||
| appWindows.set(gooseApp.name, appWindow); | ||
|
|
||
| appWindow.on('close', () => { | ||
| goosedClients.delete(appWindow.id); | ||
| appWindows.delete(gooseApp.name); |
There was a problem hiding this comment.
There's a potential race condition: if two apps have the same name and are launched simultaneously, the second launch will overwrite the first window reference in the appWindows map (line 2477), causing the first window to become untrackable. When the first window closes, it will delete the map entry (line 2481), leaving the second window's reference orphaned. Consider using a compound key (e.g., ${name}_${windowId}) or tracking windows by ID instead of name.
| goosedClients.set(appWindow.id, launchingClient); | |
| appWindows.set(gooseApp.name, appWindow); | |
| appWindow.on('close', () => { | |
| goosedClients.delete(appWindow.id); | |
| appWindows.delete(gooseApp.name); | |
| const appWindowKey = `${gooseApp.name}_${appWindow.id}`; | |
| goosedClients.set(appWindow.id, launchingClient); | |
| appWindows.set(appWindowKey, appWindow); | |
| appWindow.on('close', () => { | |
| goosedClients.delete(appWindow.id); | |
| appWindows.delete(appWindowKey); |
| use crate::agents::extension::PlatformExtensionContext; | ||
| use crate::agents::mcp_client::{Error, McpClientTrait, McpMeta}; | ||
| use crate::config::paths::Paths; | ||
| use crate::conversation::message::Message; | ||
| use crate::goose_apps::{GooseApp, WindowProps}; | ||
| use crate::goose_apps::McpAppResource; | ||
| use crate::prompt_template::render_template; | ||
| use crate::providers::base::Provider; | ||
| use async_trait::async_trait; | ||
| use rmcp::model::{ | ||
| CallToolResult, Content, Implementation, InitializeResult, JsonObject, ListResourcesResult, | ||
| ListToolsResult, Meta, ProtocolVersion, RawResource, ReadResourceResult, Resource, | ||
| ResourceContents, ResourcesCapability, ServerCapabilities, Tool as McpTool, ToolsCapability, | ||
| }; | ||
| use schemars::{schema_for, JsonSchema}; | ||
| use serde::{Deserialize, Serialize}; | ||
| use serde_json::json; | ||
| use std::collections::HashMap; | ||
| use std::fs; | ||
| use std::path::PathBuf; | ||
| use std::sync::Arc; | ||
| use tokio_util::sync::CancellationToken; | ||
|
|
||
| pub static EXTENSION_NAME: &str = "apps"; | ||
|
|
||
| const DEFAULT_WINDOW_PROPS: WindowProps = WindowProps { | ||
| width: 800, | ||
| height: 600, | ||
| resizable: true, | ||
| }; | ||
|
|
||
| #[derive(Debug, Serialize, Deserialize, JsonSchema)] | ||
| struct CreateAppParams { | ||
| /// What the app should do - a description or PRD that will be used to generate the app | ||
| prd: String, | ||
| } | ||
|
|
||
| /// Parameters for iterate_app tool | ||
| #[derive(Debug, Serialize, Deserialize, JsonSchema)] | ||
| struct IterateAppParams { | ||
| /// Name of the app to iterate on | ||
| name: String, | ||
| /// Feedback or requested changes to improve the app | ||
| feedback: String, | ||
| } | ||
|
|
||
| /// Parameters for delete_app tool | ||
| #[derive(Debug, Serialize, Deserialize, JsonSchema)] | ||
| struct DeleteAppParams { | ||
| /// Name of the app to delete | ||
| name: String, | ||
| } | ||
|
|
||
| /// Parameters for list_apps tool | ||
| #[derive(Debug, Serialize, Deserialize, JsonSchema)] | ||
| struct ListAppsParams { | ||
| // No parameters needed - lists all apps | ||
| } | ||
|
|
||
| /// Response from create_app_content tool | ||
| #[derive(Debug, Serialize, Deserialize, JsonSchema)] | ||
| struct CreateAppContentResponse { | ||
| /// App name (lowercase, hyphens allowed, no spaces) | ||
| name: String, | ||
| /// Brief description of what the app does (1-2 sentences, max 100 chars) | ||
| description: String, | ||
| /// Complete HTML code for the app, from <!DOCTYPE html> to </html> | ||
| html: String, | ||
| /// Window width in pixels (recommended: 400-1600) | ||
| width: Option<u32>, | ||
| /// Window height in pixels (recommended: 300-1200) | ||
| height: Option<u32>, | ||
| /// Whether the window should be resizable | ||
| resizable: Option<bool>, | ||
| } | ||
|
|
||
| /// Response from update_app_content tool | ||
| #[derive(Debug, Serialize, Deserialize, JsonSchema)] | ||
| struct UpdateAppContentResponse { | ||
| /// Updated description of what the app does (1-2 sentences, max 100 chars) | ||
| description: String, | ||
| /// Complete updated HTML code for the app, from <!DOCTYPE html> to </html> | ||
| html: String, | ||
| /// Updated PRD reflecting the current state of the app after this iteration | ||
| prd: String, | ||
| /// Updated window width in pixels (optional - only if size should change) | ||
| width: Option<u32>, | ||
| /// Updated window height in pixels (optional - only if size should change) | ||
| height: Option<u32>, | ||
| /// Updated resizable property (optional - only if it should change) | ||
| resizable: Option<bool>, | ||
| } | ||
|
|
||
| pub struct AppsManagerClient { | ||
| info: InitializeResult, | ||
| context: PlatformExtensionContext, | ||
| apps_dir: PathBuf, | ||
| } | ||
|
|
||
| impl AppsManagerClient { | ||
| pub fn new(context: PlatformExtensionContext) -> Result<Self, String> { | ||
| let apps_dir = Paths::in_data_dir(EXTENSION_NAME); | ||
|
|
||
| fs::create_dir_all(&apps_dir) | ||
| .map_err(|e| format!("Failed to create apps directory: {}", e))?; | ||
|
|
||
| let info = InitializeResult { | ||
| protocol_version: ProtocolVersion::V_2025_03_26, | ||
| capabilities: ServerCapabilities { | ||
| tools: Some(ToolsCapability { | ||
| list_changed: Some(false), | ||
| }), | ||
| resources: Some(ResourcesCapability { | ||
| subscribe: Some(false), | ||
| list_changed: Some(false), | ||
| }), | ||
| prompts: None, | ||
| completions: None, | ||
| experimental: None, | ||
| tasks: None, | ||
| logging: None, | ||
| }, | ||
| server_info: Implementation { | ||
| name: EXTENSION_NAME.to_string(), | ||
| title: Some("Apps Manager".to_string()), | ||
| version: "1.0.0".to_string(), | ||
| icons: None, | ||
| website_url: None, | ||
| }, | ||
| instructions: Some( | ||
| "Use this extension to create, manage, and iterate on custom HTML/CSS/JavaScript apps." | ||
| .to_string(), | ||
| ), | ||
| }; | ||
|
|
||
| Ok(Self { | ||
| info, | ||
| context, | ||
| apps_dir, | ||
| }) | ||
| } | ||
|
|
||
| fn list_stored_apps(&self) -> Result<Vec<String>, String> { | ||
| let mut apps = Vec::new(); | ||
|
|
||
| let entries = fs::read_dir(&self.apps_dir) | ||
| .map_err(|e| format!("Failed to read apps directory: {}", e))?; | ||
|
|
||
| for entry in entries { | ||
| let entry = entry.map_err(|e| format!("Failed to read directory entry: {}", e))?; | ||
| let path = entry.path(); | ||
|
|
||
| if path.extension().and_then(|s| s.to_str()) == Some("html") { | ||
| if let Some(stem) = path.file_stem().and_then(|s| s.to_str()) { | ||
| apps.push(stem.to_string()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| apps.sort(); | ||
| Ok(apps) | ||
| } | ||
|
|
||
| fn load_app(&self, name: &str) -> Result<GooseApp, String> { | ||
| let path = self.apps_dir.join(format!("{}.html", name)); | ||
|
|
||
| let html = | ||
| fs::read_to_string(&path).map_err(|e| format!("Failed to read app file: {}", e))?; | ||
|
|
||
| GooseApp::from_html(&html) | ||
| } | ||
|
|
||
| fn save_app(&self, app: &GooseApp) -> Result<(), String> { | ||
| let path = self.apps_dir.join(format!("{}.html", app.resource.name)); | ||
|
|
||
| let html_content = app.to_html()?; | ||
|
|
||
| fs::write(&path, html_content).map_err(|e| format!("Failed to write app file: {}", e))?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn delete_app(&self, name: &str) -> Result<(), String> { | ||
| let path = self.apps_dir.join(format!("{}.html", name)); | ||
|
|
||
| fs::remove_file(&path).map_err(|e| format!("Failed to delete app file: {}", e))?; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn with_platform_notification( | ||
| &self, | ||
| result: CallToolResult, | ||
| event_type: &str, | ||
| app_name: &str, | ||
| ) -> CallToolResult { | ||
| let mut params = serde_json::Map::new(); | ||
| params.insert("app_name".to_string(), json!(app_name)); | ||
| self.context | ||
| .result_with_platform_notification(result, EXTENSION_NAME, event_type, params) | ||
| } | ||
|
|
||
| async fn get_provider(&self) -> Result<Arc<dyn Provider>, String> { | ||
| let extension_manager = self | ||
| .context | ||
| .extension_manager | ||
| .as_ref() | ||
| .and_then(|weak| weak.upgrade()) | ||
| .ok_or("Extension manager not available")?; | ||
|
|
||
| let provider_guard = extension_manager.get_provider().lock().await; | ||
|
|
||
| let provider = provider_guard | ||
| .as_ref() | ||
| .ok_or("Provider not available")? | ||
| .clone(); | ||
|
|
||
| Ok(provider) | ||
| } | ||
|
|
||
| fn schema<T: JsonSchema>() -> JsonObject { | ||
| serde_json::to_value(schema_for!(T)) | ||
| .map(|v| v.as_object().unwrap().clone()) | ||
| .expect("valid schema") | ||
| } | ||
|
|
||
| fn create_app_content_tool() -> rmcp::model::Tool { | ||
| rmcp::model::Tool::new( | ||
| "create_app_content".to_string(), | ||
| "Generate content for a new Goose app. Returns the HTML code, app name, description, and window properties.".to_string(), | ||
| Self::schema::<CreateAppContentResponse>(), | ||
| ) | ||
| } | ||
|
|
||
| fn update_app_content_tool() -> rmcp::model::Tool { | ||
| rmcp::model::Tool::new( | ||
| "update_app_content".to_string(), | ||
| "Generate updated content for an existing Goose app. Returns the improved HTML code, updated description, and optionally updated window properties.".to_string(), | ||
| Self::schema::<UpdateAppContentResponse>(), | ||
| ) | ||
| } | ||
|
|
||
| async fn generate_new_app_content( | ||
| &self, | ||
| prd: &str, | ||
| ) -> Result<CreateAppContentResponse, String> { | ||
| let provider = self.get_provider().await?; | ||
|
|
||
| let existing_apps = self.list_stored_apps().unwrap_or_default(); | ||
| let existing_names = existing_apps.join(", "); | ||
|
|
||
| let context: HashMap<&str, &str> = HashMap::new(); | ||
| let system_prompt = render_template("apps_create.md", &context) | ||
| .map_err(|e| format!("Failed to render template: {}", e))?; | ||
|
|
||
| let user_prompt = format!( | ||
| "REQUESTED APP:\n{}\n\nEXISTING APPS: {}\n\nGenerate a unique name (lowercase with hyphens, not in existing apps), a brief description, complete HTML, and appropriate window size for this app.", | ||
| prd, | ||
| if existing_names.is_empty() { "none" } else { &existing_names } | ||
| ); | ||
|
|
||
| let messages = vec![Message::user().with_text(&user_prompt)]; | ||
| let tools = vec![Self::create_app_content_tool()]; | ||
|
|
||
| let (response, _usage) = provider | ||
| .complete(&system_prompt, &messages, &tools) | ||
| .await | ||
| .map_err(|e| format!("LLM call failed: {}", e))?; | ||
|
|
||
| extract_tool_response(&response, "create_app_content") | ||
| } | ||
|
|
||
| async fn generate_updated_app_content( | ||
| &self, | ||
| existing_html: &str, | ||
| existing_prd: &str, | ||
| feedback: &str, | ||
| ) -> Result<UpdateAppContentResponse, String> { | ||
| let provider = self.get_provider().await?; | ||
|
|
||
| let context: HashMap<&str, &str> = HashMap::new(); | ||
| let system_prompt = render_template("apps_iterate.md", &context) | ||
| .map_err(|e| format!("Failed to render template: {}", e))?; | ||
|
|
||
| let user_prompt = format!( | ||
| "ORIGINAL PRD:\n{}\n\nCURRENT APP:\n```html\n{}\n```\n\nFEEDBACK: {}\n\nImplement the requested changes and return:\n1. Updated description\n2. Updated HTML implementing the feedback\n3. Updated PRD reflecting the current state of the app\n4. Optionally updated window size if appropriate", | ||
| existing_prd, | ||
| existing_html, | ||
| feedback | ||
| ); | ||
|
|
||
| let messages = vec![Message::user().with_text(&user_prompt)]; | ||
| let tools = vec![Self::update_app_content_tool()]; | ||
|
|
||
| let (response, _usage) = provider | ||
| .complete(&system_prompt, &messages, &tools) | ||
| .await | ||
| .map_err(|e| format!("LLM call failed: {}", e))?; | ||
|
|
||
| extract_tool_response(&response, "update_app_content") | ||
| } | ||
|
|
||
| async fn handle_list_apps( | ||
| &self, | ||
| _arguments: Option<JsonObject>, | ||
| _meta: McpMeta, | ||
| ) -> Result<CallToolResult, String> { | ||
| let app_names = self.list_stored_apps()?; | ||
|
|
||
| if app_names.is_empty() { | ||
| return Ok(CallToolResult::success(vec![Content::text( | ||
| "No apps found. Create your first app with the create_app tool!".to_string(), | ||
| )])); | ||
| } | ||
|
|
||
| let mut apps_info = vec![format!("Found {} app(s):\n", app_names.len())]; | ||
|
|
||
| for name in app_names { | ||
| match self.load_app(&name) { | ||
| Ok(app) => { | ||
| let description = app | ||
| .resource | ||
| .description | ||
| .as_deref() | ||
| .unwrap_or("No description"); | ||
|
|
||
| let size = if let Some(ref props) = app.window_props { | ||
| format!(" ({}x{})", props.width, props.height) | ||
| } else { | ||
| String::new() | ||
| }; | ||
|
|
||
| apps_info.push(format!("- {}{}: {}", name, size, description)); | ||
| } | ||
| Err(e) => { | ||
| apps_info.push(format!("- {}: (error loading: {})", name, e)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(CallToolResult::success(vec![Content::text( | ||
| apps_info.join("\n"), | ||
| )])) | ||
| } | ||
|
|
||
| async fn handle_create_app( | ||
| &self, | ||
| arguments: Option<JsonObject>, | ||
| _meta: McpMeta, | ||
| ) -> Result<CallToolResult, String> { | ||
| let args = arguments.ok_or("Missing arguments")?; | ||
| let prd = extract_string(&args, "prd")?; | ||
|
|
||
| let content = self.generate_new_app_content(&prd).await?; | ||
|
|
||
| if self.load_app(&content.name).is_ok() { | ||
| return Err(format!( | ||
| "App '{}' already exists (generated name conflicts with existing app).", | ||
| content.name | ||
| )); | ||
| } | ||
|
|
||
| let app = GooseApp { | ||
| resource: McpAppResource { | ||
| uri: format!("ui://apps/{}", content.name), | ||
| name: content.name.clone(), | ||
| description: Some(content.description), | ||
| mime_type: "text/html;profile=mcp-app".to_string(), | ||
| text: Some(content.html), | ||
| blob: None, | ||
| meta: None, | ||
| }, | ||
| mcp_server: Some(EXTENSION_NAME.to_string()), | ||
| window_props: Some(WindowProps { | ||
| width: content.width.unwrap_or(DEFAULT_WINDOW_PROPS.width), | ||
| height: content.height.unwrap_or(DEFAULT_WINDOW_PROPS.height), | ||
| resizable: content.resizable.unwrap_or(DEFAULT_WINDOW_PROPS.resizable), | ||
| }), | ||
| prd: Some(prd), | ||
| }; | ||
|
|
||
| self.save_app(&app)?; | ||
|
|
||
| let result = CallToolResult::success(vec![Content::text(format!( | ||
| "Created app '{}'! It should have automatically opened in a new window. You can always find it again in the [Apps] tab.", | ||
| content.name | ||
| ))]); | ||
|
|
||
| Ok(self.with_platform_notification(result, "app_created", &content.name)) | ||
| } | ||
|
|
||
| async fn handle_iterate_app( | ||
| &self, | ||
| arguments: Option<JsonObject>, | ||
| _meta: McpMeta, | ||
| ) -> Result<CallToolResult, String> { | ||
| let args = arguments.ok_or("Missing arguments")?; | ||
|
|
||
| let name = extract_string(&args, "name")?; | ||
| let feedback = extract_string(&args, "feedback")?; | ||
|
|
||
| let mut app = self.load_app(&name)?; | ||
|
|
||
| let existing_html = app | ||
| .resource | ||
| .text | ||
| .as_deref() | ||
| .ok_or("App has no HTML content")?; | ||
|
|
||
| let existing_prd = app.prd.as_deref().unwrap_or(""); | ||
|
|
||
| let content = self | ||
| .generate_updated_app_content(existing_html, existing_prd, &feedback) | ||
| .await?; | ||
|
|
||
| app.resource.text = Some(content.html); | ||
| app.resource.description = Some(content.description); | ||
| app.prd = Some(content.prd); | ||
| if content.width.is_some() || content.height.is_some() || content.resizable.is_some() { | ||
| let current_props = app.window_props.as_ref(); | ||
| let default_width = current_props | ||
| .map(|p| p.width) | ||
| .unwrap_or(DEFAULT_WINDOW_PROPS.width); | ||
| let default_height = current_props | ||
| .map(|p| p.height) | ||
| .unwrap_or(DEFAULT_WINDOW_PROPS.height); | ||
| let default_resizable = current_props | ||
| .map(|p| p.resizable) | ||
| .unwrap_or(DEFAULT_WINDOW_PROPS.resizable); | ||
|
|
||
| app.window_props = Some(WindowProps { | ||
| width: content.width.unwrap_or(default_width), | ||
| height: content.height.unwrap_or(default_height), | ||
| resizable: content.resizable.unwrap_or(default_resizable), | ||
| }); | ||
| } | ||
|
|
||
| self.save_app(&app)?; | ||
|
|
||
| let result = CallToolResult::success(vec![Content::text(format!( | ||
| "Updated app '{}' based on your feedback", | ||
| name | ||
| ))]); | ||
|
|
||
| Ok(self.with_platform_notification(result, "app_updated", &name)) | ||
| } | ||
|
|
||
| async fn handle_delete_app( | ||
| &self, | ||
| arguments: Option<JsonObject>, | ||
| _meta: McpMeta, | ||
| ) -> Result<CallToolResult, String> { | ||
| let args = arguments.ok_or("Missing arguments")?; | ||
|
|
||
| let name = extract_string(&args, "name")?; | ||
|
|
||
| self.delete_app(&name)?; | ||
|
|
||
| let result = | ||
| CallToolResult::success(vec![Content::text(format!("Deleted app '{}'", name))]); | ||
|
|
||
| Ok(self.with_platform_notification(result, "app_deleted", &name)) | ||
| } | ||
| } | ||
|
|
||
| #[async_trait] | ||
| impl McpClientTrait for AppsManagerClient { | ||
| async fn list_tools( | ||
| &self, | ||
| _next_cursor: Option<String>, | ||
| _cancel_token: CancellationToken, | ||
| ) -> Result<ListToolsResult, Error> { | ||
| let tools = vec![ | ||
| McpTool::new( | ||
| "list_apps".to_string(), | ||
| "List all available Goose apps with their names and descriptions. Use this to see what apps exist before creating or modifying apps.".to_string(), | ||
| schema::<ListAppsParams>(), | ||
| ), | ||
| McpTool::new( | ||
| "create_app".to_string(), | ||
| "Create a new Goose app based on a description or PRD. The extension will use an LLM to generate the HTML/CSS/JavaScript. Apps are sandboxed and run in standalone windows.".to_string(), | ||
| schema::<CreateAppParams>(), | ||
| ), | ||
| McpTool::new( | ||
| "iterate_app".to_string(), | ||
| "Improve an existing app based on feedback. The extension will use an LLM to update the HTML while preserving the app's intent.".to_string(), | ||
| schema::<IterateAppParams>(), | ||
| ), | ||
| McpTool::new( | ||
| "delete_app".to_string(), | ||
| "Delete an app permanently".to_string(), | ||
| schema::<DeleteAppParams>(), | ||
| ), | ||
| ]; | ||
|
|
||
| Ok(ListToolsResult { | ||
| tools, | ||
| next_cursor: None, | ||
| meta: None, | ||
| }) | ||
| } | ||
|
|
||
| async fn call_tool( | ||
| &self, | ||
| name: &str, | ||
| arguments: Option<JsonObject>, | ||
| meta: McpMeta, | ||
| _cancel_token: CancellationToken, | ||
| ) -> Result<CallToolResult, Error> { | ||
| let result = match name { | ||
| "list_apps" => self.handle_list_apps(arguments, meta).await, | ||
| "create_app" => self.handle_create_app(arguments, meta).await, | ||
| "iterate_app" => self.handle_iterate_app(arguments, meta).await, | ||
| "delete_app" => self.handle_delete_app(arguments, meta).await, | ||
| _ => Err(format!("Unknown tool: {}", name)), | ||
| }; | ||
|
|
||
| match result { | ||
| Ok(result) => Ok(result), | ||
| Err(error) => Ok(CallToolResult::error(vec![Content::text(format!( | ||
| "Error: {}", | ||
| error | ||
| ))])), | ||
| } | ||
| } | ||
|
|
||
| async fn list_resources( | ||
| &self, | ||
| _next_cursor: Option<String>, | ||
| _cancel_token: CancellationToken, | ||
| ) -> Result<ListResourcesResult, Error> { | ||
| let app_names = self | ||
| .list_stored_apps() | ||
| .map_err(|_| Error::TransportClosed)?; | ||
|
|
||
| let mut resources = Vec::new(); | ||
|
|
||
| for name in app_names { | ||
| if let Ok(app) = self.load_app(&name) { | ||
| let meta = if let Some(ref window_props) = app.window_props { | ||
| let mut meta_obj = Meta::new(); | ||
| meta_obj.insert( | ||
| "window".to_string(), | ||
| json!({ | ||
| "width": window_props.width, | ||
| "height": window_props.height, | ||
| "resizable": window_props.resizable, | ||
| }), | ||
| ); | ||
| Some(meta_obj) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let raw_resource = RawResource { | ||
| uri: app.resource.uri.clone(), | ||
| name: app.resource.name.clone(), | ||
| title: None, | ||
| description: app.resource.description.clone(), | ||
| mime_type: Some(app.resource.mime_type.clone()), | ||
| size: None, | ||
| icons: None, | ||
| meta, | ||
| }; | ||
| resources.push(Resource { | ||
| raw: raw_resource, | ||
| annotations: None, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| Ok(ListResourcesResult { | ||
| resources, | ||
| next_cursor: None, | ||
| meta: None, | ||
| }) | ||
| } | ||
|
|
||
| async fn read_resource( | ||
| &self, | ||
| uri: &str, | ||
| _cancel_token: CancellationToken, | ||
| ) -> Result<ReadResourceResult, Error> { | ||
| let app_name = uri | ||
| .strip_prefix("ui://apps/") | ||
| .ok_or(Error::TransportClosed)?; | ||
|
|
||
| let app = self | ||
| .load_app(app_name) | ||
| .map_err(|_| Error::TransportClosed)?; | ||
|
|
||
| let html = app | ||
| .resource | ||
| .text | ||
| .unwrap_or_else(|| String::from("No content")); | ||
|
|
||
| Ok(ReadResourceResult { | ||
| contents: vec![ResourceContents::text(html, uri)], | ||
| }) | ||
| } | ||
|
|
||
| fn get_info(&self) -> Option<&InitializeResult> { | ||
| Some(&self.info) | ||
| } | ||
| } | ||
|
|
||
| fn schema<T: JsonSchema>() -> JsonObject { | ||
| serde_json::to_value(schema_for!(T)) | ||
| .map(|v| v.as_object().unwrap().clone()) | ||
| .expect("valid schema") | ||
| } | ||
|
|
||
| fn extract_string(args: &JsonObject, key: &str) -> Result<String, String> { | ||
| args.get(key) | ||
| .and_then(|v| v.as_str()) | ||
| .map(|s| s.to_string()) | ||
| .ok_or_else(|| format!("Missing or invalid '{}'", key)) | ||
| } | ||
|
|
||
| fn extract_tool_response<T: serde::de::DeserializeOwned>( | ||
| response: &Message, | ||
| tool_name: &str, | ||
| ) -> Result<T, String> { | ||
| for content in &response.content { | ||
| if let crate::conversation::message::MessageContent::ToolRequest(tool_req) = content { | ||
| if let Ok(tool_call) = &tool_req.tool_call { | ||
| if tool_call.name == tool_name { | ||
| let params = tool_call | ||
| .arguments | ||
| .as_ref() | ||
| .ok_or("Missing tool call parameters")?; | ||
|
|
||
| return serde_json::from_value(serde_json::Value::Object(params.clone())) | ||
| .map_err(|e| format!("Failed to parse tool response: {}", e)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Err(format!("LLM did not call the required tool: {}", tool_name)) | ||
| } |
There was a problem hiding this comment.
The apps_extension.rs module lacks test coverage. Other extensions in the codebase (e.g., code_execution_extension.rs, skills_extension.rs) include comprehensive tests. Consider adding tests for critical functionality including: app creation/update/delete operations, HTML parsing (from_html/to_html), path sanitization, and platform notification generation.
|
|
||
| use super::app::GooseApp; | ||
|
|
||
| static CLOCK_HTML: &str = include_str!("../goose_apps/clock.html"); |
There was a problem hiding this comment.
Incorrect path in include_str macro. The path "../goose_apps/clock.html" is relative to cache.rs, which is already in the goose_apps directory. This should be "./clock.html" or "clock.html".
| static CLOCK_HTML: &str = include_str!("../goose_apps/clock.html"); | |
| static CLOCK_HTML: &str = include_str!("clock.html"); |
There was a problem hiding this comment.
yeah I did see this was an issue last I tried it
| .map(|v| v.as_object().unwrap().clone()) | ||
| .expect("valid schema") |
There was a problem hiding this comment.
This unwrap will panic if the schema doesn't serialize to a JSON object. Consider returning a Result or using expect with a descriptive message instead of relying on an expect inside the closure.
| .map(|v| v.as_object().unwrap().clone()) | |
| .expect("valid schema") | |
| .and_then(|v| { | |
| v.as_object() | |
| .cloned() | |
| .ok_or_else(|| serde_json::Error::custom("schema must serialize to a JSON object")) | |
| }) | |
| .expect("failed to serialize JSON schema for MCP tool") |
| serde_json::to_value(schema_for!(T)) | ||
| .map(|v| v.as_object().unwrap().clone()) | ||
| .expect("valid schema") | ||
| } | ||
|
|
There was a problem hiding this comment.
The schema function is duplicated - it's defined both as a method at line 221 and as a standalone function at line 607. Consider removing the duplication and using only one implementation.
| serde_json::to_value(schema_for!(T)) | |
| .map(|v| v.as_object().unwrap().clone()) | |
| .expect("valid schema") | |
| } | |
| AppsManagerClient::schema::<T>() | |
| } |
| let original_name = app.resource.name.clone(); | ||
| let mut counter = 1; | ||
|
|
||
| let existing_apps = cache.list_apps().unwrap_or_default(); | ||
| let existing_names: HashSet<String> = existing_apps | ||
| .iter() | ||
| .map(|a| a.resource.name.clone()) | ||
| .collect(); | ||
|
|
||
| while existing_names.contains(&app.resource.name) { | ||
| app.resource.name = format!("{}_{}", original_name, counter); | ||
| app.resource.uri = format!("ui://apps/{}", app.resource.name); | ||
| counter += 1; | ||
| } |
There was a problem hiding this comment.
The counter-based naming strategy could result in names like "my-app_1_1_1" if an app is imported multiple times and then renamed. Consider using a UUID suffix or timestamp instead for cleaner naming.
| ctx.fillStyle = 'black'; | ||
| ctx.textAlign = 'center'; | ||
| ctx.textBaseline = 'middle'; | ||
| ctx.fillText('GOOSE CLOCK', centerX, centerY - 65); |
There was a problem hiding this comment.
The prompt refers to "goose" (lowercase) which follows the project naming convention documented in HOWTOAI.md. However, "GOOSE CLOCK" at line 174 of clock.html uses all caps which is inconsistent with the convention that "goose" should always be lowercase.
| let mut app = GooseApp::from_html(&body.html).map_err(|e| ErrorResponse { | ||
| message: format!("Invalid Goose App HTML: {}", e), | ||
| status: StatusCode::BAD_REQUEST, | ||
| })?; |
There was a problem hiding this comment.
Missing input validation on the HTML content. The function should validate that the HTML contains required metadata before attempting to parse it, and should sanitize the HTML to prevent XSS or injection attacks when it's later served to users.
| let name = metadata | ||
| .get("name") | ||
| .and_then(|v| v.as_str()) | ||
| .ok_or("Missing 'name' in metadata")? | ||
| .to_string(); | ||
|
|
There was a problem hiding this comment.
The name field extracted from untrusted HTML is not validated. It should be validated to ensure it only contains safe characters (lowercase letters, numbers, hyphens) and doesn't contain path traversal sequences or other malicious patterns before being used in file paths.
| let name = metadata | |
| .get("name") | |
| .and_then(|v| v.as_str()) | |
| .ok_or("Missing 'name' in metadata")? | |
| .to_string(); | |
| let name_str = metadata | |
| .get("name") | |
| .and_then(|v| v.as_str()) | |
| .ok_or("Missing 'name' in metadata")?; | |
| // Validate that the name only contains safe characters to avoid path traversal or injection. | |
| let name_validator = | |
| Regex::new(r"^[a-z0-9-]+$").map_err(|e| format!("Regex error: {}", e))?; | |
| if !name_validator.is_match(name_str) { | |
| return Err("Invalid 'name' in metadata: only lowercase letters, numbers, and hyphens are allowed" | |
| .to_string()); | |
| } | |
| let name = name_str.to_string(); |
| if self.load_app(&content.name).is_ok() { | ||
| return Err(format!( | ||
| "App '{}' already exists (generated name conflicts with existing app).", | ||
| content.name |
There was a problem hiding this comment.
The error message formats a dynamic app name that could contain special characters from untrusted input. While this is just an error message, consider sanitizing the app name before including it in user-facing text to prevent potential injection issues in logs or UI.
| * Check if a notification is a platform event and dispatch it as a window CustomEvent. | ||
| * Called from useChatStream when receiving Notification MessageEvents. | ||
| */ | ||
| export function maybe_handle_platform_event(notification: unknown, sessionId: string): void { |
There was a problem hiding this comment.
This function uses snake_case naming which is inconsistent with TypeScript/JavaScript conventions. Should be renamed to maybeHandlePlatformEvent to follow camelCase convention.
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
I think this is a good foot hold - not sure if we need to label it preview but if can be updated, I don't see downside for this to at least start and then iterate
|
@DOsinga not sure how hallucinated the copilot feedback is but some may be legit |
| fn cache_key(extension_name: &str, resource_uri: &str) -> String { | ||
| let input = format!("{}::{}", extension_name, resource_uri); | ||
| let hash = Sha256::digest(input.as_bytes()); | ||
| format!("{}_{:x}", extension_name, hash) | ||
| } |
There was a problem hiding this comment.
The cache key generation uses SHA256 hashing but converts only the first few bytes to hex. The format string {:x} formats the entire hash result, but the variable naming suggests only a partial hash is intended. If this is meant to be a unique identifier, consider using the full hash or explicitly specifying how many bytes to use.
| let metadata_re = Regex::new(&format!( | ||
| r#"(?s)<script type="{}"[^>]*>\s*(.*?)\s*</script>"#, | ||
| regex::escape(Self::METADATA_SCRIPT_TYPE) | ||
| )) | ||
| .map_err(|e| format!("Regex error: {}", e))?; | ||
|
|
||
| let prd_re = Regex::new(&format!( | ||
| r#"(?s)<script type="{}"[^>]*>\s*(.*?)\s*</script>"#, | ||
| regex::escape(Self::PRD_SCRIPT_TYPE) | ||
| )) | ||
| .map_err(|e| format!("Regex error: {}", e))?; |
There was a problem hiding this comment.
The regex patterns are compiled on every call to from_html. Consider compiling these regexes once using lazy_static or OnceCell to improve performance when parsing multiple apps.
|
|
||
| fn schema<T: JsonSchema>() -> JsonObject { | ||
| serde_json::to_value(schema_for!(T)) | ||
| .map(|v| v.as_object().unwrap().clone()) |
There was a problem hiding this comment.
The unwrap call on line 223 can panic if the schema fails to convert to an object. While unlikely, this should use expect with a descriptive message or handle the error more gracefully.
| .map(|v| v.as_object().unwrap().clone()) | |
| .map(|v| v.as_object().expect("schema_for!(T) must serialize to a JSON object").clone()) |
| export function registerPlatformEventHandlers(): () => void { | ||
| const handler = (event: Event) => { | ||
| const customEvent = event as CustomEvent; | ||
| const { extension, event_type, ...data } = customEvent.detail; | ||
|
|
||
| const extensionHandler = EXTENSION_HANDLERS[extension]; | ||
| if (extensionHandler) { | ||
| extensionHandler(event_type, { ...data, extension }).catch((err) => { | ||
| console.error(`Platform event handler failed for ${extension}:`, err); | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener('platform-event', handler); | ||
| return () => window.removeEventListener('platform-event', handler); | ||
| } |
There was a problem hiding this comment.
The event listener cleanup function is returned but there's no guarantee it will be called if the component unmounts abnormally. While the useEffect cleanup handles normal cases, consider whether additional cleanup is needed for edge cases.
| fn schema<T: JsonSchema>() -> JsonObject { | ||
| serde_json::to_value(schema_for!(T)) | ||
| .map(|v| v.as_object().unwrap().clone()) | ||
| .expect("valid schema") | ||
| } |
There was a problem hiding this comment.
This duplicates the schema function defined at line 607. Consider removing this duplicate and using the module-level function instead.
| fn list_stored_apps(&self) -> Result<Vec<String>, String> { | ||
| let mut apps = Vec::new(); | ||
|
|
||
| let entries = fs::read_dir(&self.apps_dir) | ||
| .map_err(|e| format!("Failed to read apps directory: {}", e))?; |
There was a problem hiding this comment.
The error messages returned to users may expose internal file system paths. Consider sanitizing error messages to avoid potential information disclosure, especially for the app cache directory structure.
| async fn read_resource( | ||
| &self, | ||
| uri: &str, | ||
| _cancel_token: CancellationToken, | ||
| ) -> Result<ReadResourceResult, Error> { | ||
| let app_name = uri | ||
| .strip_prefix("ui://apps/") | ||
| .ok_or(Error::TransportClosed)?; | ||
|
|
||
| let app = self | ||
| .load_app(app_name) | ||
| .map_err(|_| Error::TransportClosed)?; | ||
|
|
||
| let html = app | ||
| .resource | ||
| .text | ||
| .unwrap_or_else(|| String::from("No content")); | ||
|
|
||
| Ok(ReadResourceResult { | ||
| contents: vec![ResourceContents::text(html, uri)], | ||
| }) | ||
| } |
There was a problem hiding this comment.
The error type uses Error::TransportClosed as a catch-all error, which doesn't accurately describe the actual error conditions (app not found, invalid URI). Consider adding more specific error variants or using a more appropriate existing variant.
| match fs::read_to_string(&path) { | ||
| Ok(content) => match serde_json::from_str::<GooseApp>(&content) { | ||
| Ok(app) => apps.push(app), | ||
| Err(e) => warn!("Failed to parse cached app from {:?}: {}", path, e), | ||
| }, | ||
| Err(e) => warn!("Failed to read cached app from {:?}: {}", path, e), | ||
| } |
There was a problem hiding this comment.
The error handling silently continues on parse failures, which could hide issues with cached apps. Consider logging warnings at a higher severity or implementing a recovery mechanism for corrupted cache entries.
| - Use vanilla JavaScript (no external dependencies unless absolutely necessary) | ||
| - If you need external resources (fonts, icons), use CDN links | ||
| - The app will be sandboxed with strict CSP, so all scripts must be inline or from trusted CDNs |
There was a problem hiding this comment.
The prompt template includes instructions to "Use vanilla JavaScript (no external dependencies unless absolutely necessary)" but also allows CDN links. This creates ambiguity - the LLM might interpret "absolutely necessary" differently. Consider being more explicit about when CDN usage is acceptable.
| - Use vanilla JavaScript (no external dependencies unless absolutely necessary) | |
| - If you need external resources (fonts, icons), use CDN links | |
| - The app will be sandboxed with strict CSP, so all scripts must be inline or from trusted CDNs | |
| - Use vanilla JavaScript; do not load external JavaScript libraries (no JS dependencies from CDNs or packages) | |
| - If you need external resources (fonts, icons, or CSS only), use CDN links from well-known, trusted providers | |
| - The app will be sandboxed with strict CSP, so all JavaScript must be inline; only non-script assets (fonts, icons, CSS) may be loaded from trusted CDNs |
| console.warn('No sessionId in apps platform event, skipping'); | ||
| return; |
There was a problem hiding this comment.
The function silently returns early if no sessionId is provided, but this is logged at the warn level. Consider if this should be an error condition that throws or is handled differently, since platform events without a session context may indicate a larger issue.
| console.warn('No sessionId in apps platform event, skipping'); | |
| return; | |
| console.error('No sessionId in apps platform event; unable to handle apps event'); | |
| throw new Error('Missing sessionId in apps platform event'); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| fn load_app(&self, name: &str) -> Result<GooseApp, String> { | ||
| let path = self.apps_dir.join(format!("{}.html", name)); | ||
|
|
||
| let html = | ||
| fs::read_to_string(&path).map_err(|e| format!("Failed to read app file: {}", e))?; | ||
|
|
||
| GooseApp::from_html(&html) | ||
| } |
There was a problem hiding this comment.
The error handling converts all errors to generic string messages. The load_app function returns a string error, but it loses the structured error information from the underlying fs::read_to_string or from_html operations. This makes debugging harder. Consider using a proper error type with context about what failed.
| if let Ok(mut clock_app) = GooseApp::from_html(CLOCK_HTML) { | ||
| clock_app.mcp_servers = vec![APPS_EXTENSION_NAME.to_string()]; | ||
| let _ = self.store_app(&clock_app); |
There was a problem hiding this comment.
The ensure_default_apps method is called synchronously during cache initialization but silently ignores all errors. If the clock app fails to cache (e.g., due to disk permission issues), the user won't know. At minimum, log the error for debugging purposes.
| if let Ok(mut clock_app) = GooseApp::from_html(CLOCK_HTML) { | |
| clock_app.mcp_servers = vec![APPS_EXTENSION_NAME.to_string()]; | |
| let _ = self.store_app(&clock_app); | |
| warn!( | |
| "Default clock app not found in cache; attempting to create and store it" | |
| ); | |
| match GooseApp::from_html(CLOCK_HTML) { | |
| Ok(mut clock_app) => { | |
| clock_app.mcp_servers = vec![APPS_EXTENSION_NAME.to_string()]; | |
| if let Err(e) = self.store_app(&clock_app) { | |
| warn!("Failed to store default clock app in cache: {}", e); | |
| } | |
| } | |
| Err(e) => { | |
| warn!("Failed to create default clock app from embedded HTML: {}", e); | |
| } |
| ref={fileInputRef} | ||
| type="file" | ||
| accept=".html" | ||
| onChange={handleUploadApp} | ||
| style={{ display: 'none' }} |
There was a problem hiding this comment.
Missing accessibility: The file input for importing apps is hidden with style={{ display: 'none' }}, which prevents keyboard navigation. While the button triggers it, this pattern doesn't provide accessible file selection. Consider using proper accessible hidden patterns or ensuring the button properly conveys the file input's purpose to screen readers.
| ref={fileInputRef} | |
| type="file" | |
| accept=".html" | |
| onChange={handleUploadApp} | |
| style={{ display: 'none' }} | |
| id="import-app-file-input" | |
| ref={fileInputRef} | |
| type="file" | |
| accept=".html" | |
| onChange={handleUploadApp} | |
| className="sr-only" | |
| tabIndex={-1} | |
| aria-hidden="true" |
| <Button | ||
| variant="outline" | ||
| size="sm" | ||
| onClick={() => handleDownloadApp(app)} | ||
| className="flex items-center gap-2" | ||
| > | ||
| <Download className="h-4 w-4" /> | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
The download button for custom apps doesn't have an accessible label. Screen readers will only announce "Download" (from the icon), which lacks context about what's being downloaded. Add an aria-label attribute like "Download app" or include visible text alongside the icon.
| </div> | ||
| <div className="mb-4"> | ||
| <p className="text-sm text-text-muted mb-2"> | ||
| Applications from your MCP servers and Apps build by goose itself. You can ask it to |
There was a problem hiding this comment.
Grammatical error: "Apps build by goose" should be "Apps built by goose".
| Applications from your MCP servers and Apps build by goose itself. You can ask it to | |
| Applications from your MCP servers and Apps built by goose itself. You can ask it to |
|
|
||
| let metadata_re = Regex::new(&format!( | ||
| r#"(?s)<script type="{}"[^>]*>\s*(.*?)\s*</script>"#, | ||
| regex::escape(Self::METADATA_SCRIPT_TYPE) | ||
| )) | ||
| .map_err(|e| format!("Regex error: {}", e))?; | ||
|
|
||
| let prd_re = Regex::new(&format!( | ||
| r#"(?s)<script type="{}"[^>]*>\s*(.*?)\s*</script>"#, | ||
| regex::escape(Self::PRD_SCRIPT_TYPE) | ||
| )) | ||
| .map_err(|e| format!("Regex error: {}", e))?; | ||
|
|
||
| let json_str = metadata_re | ||
| .captures(html) | ||
| .and_then(|cap| cap.get(1)) | ||
| .ok_or_else(|| "No GooseApp JSON-LD metadata found in HTML".to_string())? | ||
| .as_str(); | ||
|
|
||
| let metadata: serde_json::Value = serde_json::from_str(json_str) |
There was a problem hiding this comment.
The regex parsing approach for extracting metadata from HTML is fragile and could fail with valid HTML that has different whitespace or formatting. Consider using a proper HTML parser library that can handle variations in formatting, or document strict formatting requirements for the HTML.
| let metadata_re = Regex::new(&format!( | |
| r#"(?s)<script type="{}"[^>]*>\s*(.*?)\s*</script>"#, | |
| regex::escape(Self::METADATA_SCRIPT_TYPE) | |
| )) | |
| .map_err(|e| format!("Regex error: {}", e))?; | |
| let prd_re = Regex::new(&format!( | |
| r#"(?s)<script type="{}"[^>]*>\s*(.*?)\s*</script>"#, | |
| regex::escape(Self::PRD_SCRIPT_TYPE) | |
| )) | |
| .map_err(|e| format!("Regex error: {}", e))?; | |
| let json_str = metadata_re | |
| .captures(html) | |
| .and_then(|cap| cap.get(1)) | |
| .ok_or_else(|| "No GooseApp JSON-LD metadata found in HTML".to_string())? | |
| .as_str(); | |
| let metadata: serde_json::Value = serde_json::from_str(json_str) | |
| use scraper::{Html, Selector}; | |
| let document = Html::parse_document(html); | |
| let metadata_selector = Selector::parse(&format!( | |
| r#"script[type="{}"]"#, | |
| Self::METADATA_SCRIPT_TYPE | |
| )) | |
| .map_err(|e| format!("Failed to parse metadata selector: {}", e))?; | |
| let json_str = document | |
| .select(&metadata_selector) | |
| .next() | |
| .map(|element| element.text().collect::<String>()) | |
| .map(|text| text.trim().to_string()) | |
| .ok_or_else(|| "No GooseApp JSON-LD metadata found in HTML".to_string())?; | |
| let prd_re = Regex::new(&format!( | |
| r#"(?s)<script type="{}"[^>]*>\s*(.*?)\s*</script>"#, | |
| regex::escape(Self::PRD_SCRIPT_TYPE) | |
| )) | |
| .map_err(|e| format!("Regex error: {}", e))?; | |
| let metadata: serde_json::Value = serde_json::from_str(&json_str) |
| fn save_app(&self, app: &GooseApp) -> Result<(), String> { | ||
| let path = self.apps_dir.join(format!("{}.html", app.resource.name)); |
There was a problem hiding this comment.
Missing validation: The app name isn't validated for filesystem safety. Names containing path separators, special characters, or dots could cause security issues or file system errors when saving to disk. Add validation to ensure the app name is a safe filename (e.g., alphanumeric with hyphens/underscores only).
| let result = if let Some(head_pos) = html.find("</head>") { | ||
| let mut result = html.clone(); | ||
| result.insert_str(head_pos, &scripts); | ||
| result | ||
| } else if let Some(html_pos) = html.find("<html") { | ||
| let after_html = html | ||
| .get(html_pos..) | ||
| .and_then(|s| s.find('>')) | ||
| .map(|p| html_pos + p + 1); | ||
| if let Some(pos) = after_html { | ||
| let mut result = html.clone(); | ||
| result.insert_str(pos, &format!("\n<head>\n{}</head>", scripts)); | ||
| result | ||
| } else { | ||
| format!("<head>\n{}</head>\n{}", scripts, html) | ||
| } | ||
| } else { | ||
| format!( | ||
| "<html>\n<head>\n{}</head>\n<body>\n{}\n</body>\n</html>", | ||
| scripts, html | ||
| ) | ||
| }; |
There was a problem hiding this comment.
The HTML insertion logic has a complex fallback chain but doesn't handle the case where the HTML might already contain metadata scripts. This could result in duplicate metadata blocks if to_html() is called multiple times on the same app, or if the input HTML already contains the scripts. Consider checking for existing metadata scripts before insertion.
| async function handleAppsEvent(eventType: string, eventData: PlatformEventData): Promise<void> { | ||
| const { app_name, sessionId } = eventData as AppsEventData; | ||
|
|
||
| console.log(`[platform_events] Handling apps event: ${eventType}, app_name: '${app_name}'`); | ||
|
|
||
| if (!sessionId) { | ||
| console.warn('No sessionId in apps platform event, skipping'); | ||
| return; | ||
| } | ||
|
|
||
| // Fetch fresh apps list to get latest state | ||
| const response = await listApps({ | ||
| throwOnError: false, | ||
| query: { session_id: sessionId }, | ||
| }); | ||
|
|
||
| const apps = response.data?.apps || []; | ||
| console.log( | ||
| `[platform_events] Fetched ${apps.length} apps:`, | ||
| apps.map((a: GooseApp) => a.name) | ||
| ); | ||
|
|
||
| const targetApp = apps.find((app: GooseApp) => app.name === app_name); | ||
| console.log(`[platform_events] Target app found:`, targetApp ? 'YES' : 'NO'); | ||
|
|
||
| switch (eventType) { | ||
| case 'app_created': | ||
| // Open the newly created app | ||
| if (targetApp) { | ||
| await window.electron.launchApp(targetApp).catch((err) => { | ||
| console.error('Failed to launch newly created app:', err); | ||
| }); | ||
| } | ||
| break; | ||
|
|
||
| case 'app_updated': | ||
| // Refresh the app if it's currently open | ||
| if (targetApp) { | ||
| await window.electron.refreshApp(targetApp).catch((err) => { | ||
| console.error('Failed to refresh updated app:', err); | ||
| }); | ||
| } | ||
| break; | ||
|
|
||
| case 'app_deleted': | ||
| // Close the app if it's currently open | ||
| if (app_name) { | ||
| await window.electron.closeApp(app_name).catch((err) => { | ||
| console.error('Failed to close deleted app:', err); | ||
| }); | ||
| } | ||
| break; | ||
|
|
||
| default: | ||
| console.warn(`Unknown apps event type: ${eventType}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential race condition: When multiple platform events arrive rapidly for the same app (e.g., created then immediately updated), the window management operations (launch, refresh, close) could execute out of order. The event handling is async but doesn't coordinate between events for the same app. Consider adding event sequencing or locking per app name.
| let app = apps | ||
| .into_iter() | ||
| .find(|a| a.resource.name == name) | ||
| .ok_or_else(|| ErrorResponse { | ||
| message: format!("App '{}' not found", name), | ||
| status: StatusCode::NOT_FOUND, | ||
| })?; | ||
|
|
||
| let html = app.to_html().map_err(|e| ErrorResponse { | ||
| message: format!("Failed to generate HTML: {}", e), | ||
| status: StatusCode::INTERNAL_SERVER_ERROR, | ||
| })?; |
There was a problem hiding this comment.
This error handling could be improved. If app.to_html() fails but the app object is malformed rather than missing, the user gets a "not found" error instead of a more accurate error message. Consider checking if the app exists first and returning different error messages for "app not found" vs "failed to export".
| Launch | ||
| </Button> | ||
| {apps.map((app) => { | ||
| const isCustomApp = app.mcpServers?.includes('apps') ?? false; |
There was a problem hiding this comment.
The hardcoded string "apps" is used as an extension name filter. Consider using the EXTENSION_NAME constant defined in the apps_extension module for consistency.
| </div> | ||
| <div className="mb-4"> | ||
| <p className="text-sm text-text-muted mb-2"> | ||
| Applications from your MCP servers and Apps build by goose itself. You can ask it to |
There was a problem hiding this comment.
The text refers to "Apps build by goose" which should be "Apps built by goose".
| Applications from your MCP servers and Apps build by goose itself. You can ask it to | |
| Applications from your MCP servers and Apps built by goose itself. You can ask it to |
| } | ||
|
|
||
| fn ensure_default_apps(&self) -> Result<(), String> { | ||
| // TODO(Douwe): we have the same check in cache, consider unfiying that |
There was a problem hiding this comment.
The TODO comment references "Douwe" which appears to be a developer name. Consider either implementing the unification or removing the TODO if it's not a priority, as developer-specific TODOs can become stale.
| // TODO(Douwe): we have the same check in cache, consider unfiying that | |
| // TODO: This check is duplicated in the cache module; consider unifying the implementations. |
|
|
||
| fn schema<T: JsonSchema>() -> JsonObject { | ||
| serde_json::to_value(schema_for!(T)) | ||
| .map(|v| v.as_object().unwrap().clone()) |
There was a problem hiding this comment.
The .unwrap() call will panic if the schema cannot be converted to a JSON object. Use expect with a descriptive message or handle the error properly to avoid runtime panics.
| .map(|v| v.as_object().unwrap().clone()) | |
| .map(|v| v.as_object().cloned().expect("schema JSON must be an object")) |
| fn schema<T: JsonSchema>() -> JsonObject { | ||
| serde_json::to_value(schema_for!(T)) | ||
| .map(|v| v.as_object().expect("schema_for!(T) must serialize to a JSON object").clone()) | ||
| .expect("Schema serialization must succeed") | ||
| } | ||
|
|
||
| fn create_app_content_tool() -> rmcp::model::Tool { | ||
| rmcp::model::Tool::new( | ||
| "create_app_content".to_string(), | ||
| "Generate content for a new Goose app. Returns the HTML code, app name, description, and window properties.".to_string(), | ||
| Self::schema::<CreateAppContentResponse>(), | ||
| ) | ||
| } | ||
|
|
||
| fn update_app_content_tool() -> rmcp::model::Tool { | ||
| rmcp::model::Tool::new( | ||
| "update_app_content".to_string(), | ||
| "Generate updated content for an existing Goose app. Returns the improved HTML code, updated description, and optionally updated window properties.".to_string(), | ||
| Self::schema::<UpdateAppContentResponse>(), |
There was a problem hiding this comment.
The .unwrap() call will panic if the schema doesn't serialize to an object. This is part of the schema generation code path. Use expect with a descriptive message or consider whether this should return a Result instead.
| fn schema<T: JsonSchema>() -> JsonObject { | |
| serde_json::to_value(schema_for!(T)) | |
| .map(|v| v.as_object().expect("schema_for!(T) must serialize to a JSON object").clone()) | |
| .expect("Schema serialization must succeed") | |
| } | |
| fn create_app_content_tool() -> rmcp::model::Tool { | |
| rmcp::model::Tool::new( | |
| "create_app_content".to_string(), | |
| "Generate content for a new Goose app. Returns the HTML code, app name, description, and window properties.".to_string(), | |
| Self::schema::<CreateAppContentResponse>(), | |
| ) | |
| } | |
| fn update_app_content_tool() -> rmcp::model::Tool { | |
| rmcp::model::Tool::new( | |
| "update_app_content".to_string(), | |
| "Generate updated content for an existing Goose app. Returns the improved HTML code, updated description, and optionally updated window properties.".to_string(), | |
| Self::schema::<UpdateAppContentResponse>(), | |
| fn schema<T: JsonSchema>() -> Result<JsonObject, String> { | |
| let value = serde_json::to_value(schema_for!(T)) | |
| .map_err(|e| format!("Failed to serialize schema for type: {}", e))?; | |
| let object = value | |
| .as_object() | |
| .ok_or_else(|| "schema_for!(T) did not serialize to a JSON object".to_string())?; | |
| Ok(object.clone()) | |
| } | |
| fn create_app_content_tool() -> rmcp::model::Tool { | |
| let schema = Self::schema::<CreateAppContentResponse>() | |
| .unwrap_or_else(|_| JsonObject::new()); | |
| rmcp::model::Tool::new( | |
| "create_app_content".to_string(), | |
| "Generate content for a new Goose app. Returns the HTML code, app name, description, and window properties.".to_string(), | |
| schema, | |
| ) | |
| } | |
| fn update_app_content_tool() -> rmcp::model::Tool { | |
| let schema = Self::schema::<UpdateAppContentResponse>() | |
| .unwrap_or_else(|_| JsonObject::new()); | |
| rmcp::model::Tool::new( | |
| "update_app_content".to_string(), | |
| "Generate updated content for an existing Goose app. Returns the improved HTML code, updated description, and optionally updated window properties.".to_string(), | |
| schema, |
| path: '/apps', | ||
| label: 'Apps', | ||
| icon: AppWindow, | ||
| tooltip: 'MCP and custom apps', |
There was a problem hiding this comment.
The navigation comment describes "Apps" tooltip text changing from "Browse and launch MCP apps" to "MCP and custom apps". The new text is less clear about what the page does - it doesn't mention browsing or launching, which are key user actions.
| tooltip: 'MCP and custom apps', | |
| tooltip: 'Browse and launch MCP and custom apps', |
| counter += 1; | ||
| } | ||
|
|
||
| app.mcp_servers = vec!["apps".to_string()]; |
There was a problem hiding this comment.
The hardcoded string "apps" should use a constant. This same value is defined as EXTENSION_NAME in apps_extension.rs and APPS_EXTENSION_NAME in cache.rs.
| const customEvent = event as CustomEvent; | ||
| const eventData = customEvent.detail; | ||
|
|
||
| if (eventData?.extension === 'apps') { |
There was a problem hiding this comment.
The hardcoded string "apps" should match the extension name used throughout. Consider defining this as a constant that can be imported in both frontend and backend code.
| width: gooseApp.width ?? 800, | ||
| height: gooseApp.height ?? 600, | ||
| resizable: gooseApp.resizable ?? true, | ||
| useContentSize: true, |
There was a problem hiding this comment.
The window property useContentSize is set to true but this change isn't mentioned in any documentation. This property affects how window dimensions are calculated and could cause unexpected behavior if window size calculations elsewhere assume the default behavior.
| useContentSize: true, |
|
|
||
| const workingDir = app.getPath('home'); | ||
| const extensionName = gooseApp.mcpServer ?? ''; | ||
| const extensionName = gooseApp.mcpServers?.[0] ?? ''; |
There was a problem hiding this comment.
Changed from singular mcpServer to plural mcpServers but the old code used optional chaining ?? which safely handled undefined. The new code uses ?.[0] which works, but this breaking change to the GooseApp schema could affect any external consumers or stored data. Consider if migration logic is needed for existing cached apps.
| const extensionName = gooseApp.mcpServers?.[0] ?? ''; | |
| const legacyMcpServer = (gooseApp as any).mcpServer as string | undefined; | |
| const extensionName = gooseApp.mcpServers?.[0] ?? legacyMcpServer ?? ''; |
| let metadata_re = Regex::new(&format!( | ||
| r#"(?s)<script type="{}"[^>]*>\s*(.*?)\s*</script>"#, | ||
| regex::escape(Self::METADATA_SCRIPT_TYPE) | ||
| )) | ||
| .map_err(|e| format!("Regex error: {}", e))?; | ||
|
|
||
| let prd_re = Regex::new(&format!( | ||
| r#"(?s)<script type="{}"[^>]*>\s*(.*?)\s*</script>"#, | ||
| regex::escape(Self::PRD_SCRIPT_TYPE) | ||
| )) | ||
| .map_err(|e| format!("Regex error: {}", e))?; |
There was a problem hiding this comment.
The regex construction error returns a string, but the actual parsing error would be a programming error (invalid regex pattern). This should be unreachable or panic since the regex pattern is static. Consider using a lazy_static or once_cell for compiled regex patterns instead.
| fn schema<T: JsonSchema>() -> JsonObject { | ||
| serde_json::to_value(schema_for!(T)) | ||
| .map(|v| { | ||
| v.as_object() | ||
| .expect("schema_for!(T) must serialize to a JSON object") | ||
| .clone() | ||
| }) | ||
| .expect("Schema serialization must succeed") | ||
| } | ||
|
|
||
| fn create_app_content_tool() -> rmcp::model::Tool { | ||
| rmcp::model::Tool::new( | ||
| "create_app_content".to_string(), | ||
| "Generate content for a new Goose app. Returns the HTML code, app name, description, and window properties.".to_string(), | ||
| Self::schema::<CreateAppContentResponse>(), | ||
| ) | ||
| } | ||
|
|
||
| fn update_app_content_tool() -> rmcp::model::Tool { | ||
| rmcp::model::Tool::new( | ||
| "update_app_content".to_string(), | ||
| "Generate updated content for an existing Goose app. Returns the improved HTML code, updated description, and optionally updated window properties.".to_string(), | ||
| Self::schema::<UpdateAppContentResponse>(), |
There was a problem hiding this comment.
The unwrap() calls here will panic if schema serialization fails. Since this is a compile-time constant schema, consider using expect() with a descriptive message or handle the error more gracefully.
| fn schema<T: JsonSchema>() -> JsonObject { | |
| serde_json::to_value(schema_for!(T)) | |
| .map(|v| { | |
| v.as_object() | |
| .expect("schema_for!(T) must serialize to a JSON object") | |
| .clone() | |
| }) | |
| .expect("Schema serialization must succeed") | |
| } | |
| fn create_app_content_tool() -> rmcp::model::Tool { | |
| rmcp::model::Tool::new( | |
| "create_app_content".to_string(), | |
| "Generate content for a new Goose app. Returns the HTML code, app name, description, and window properties.".to_string(), | |
| Self::schema::<CreateAppContentResponse>(), | |
| ) | |
| } | |
| fn update_app_content_tool() -> rmcp::model::Tool { | |
| rmcp::model::Tool::new( | |
| "update_app_content".to_string(), | |
| "Generate updated content for an existing Goose app. Returns the improved HTML code, updated description, and optionally updated window properties.".to_string(), | |
| Self::schema::<UpdateAppContentResponse>(), | |
| fn schema<T: JsonSchema>() -> Result<JsonObject, String> { | |
| let value = serde_json::to_value(schema_for!(T)) | |
| .map_err(|e| format!("Failed to serialize JSON schema: {e}"))?; | |
| let object = value | |
| .as_object() | |
| .ok_or_else(|| "schema_for!(T) did not serialize to a JSON object".to_string())? | |
| .clone(); | |
| Ok(object) | |
| } | |
| fn create_app_content_tool() -> rmcp::model::Tool { | |
| let schema = Self::schema::<CreateAppContentResponse>().unwrap_or_else(|err| { | |
| eprintln!("Failed to generate JSON schema for create_app_content tool: {err}"); | |
| JsonObject::default() | |
| }); | |
| rmcp::model::Tool::new( | |
| "create_app_content".to_string(), | |
| "Generate content for a new Goose app. Returns the HTML code, app name, description, and window properties.".to_string(), | |
| schema, | |
| ) | |
| } | |
| fn update_app_content_tool() -> rmcp::model::Tool { | |
| let schema = Self::schema::<UpdateAppContentResponse>().unwrap_or_else(|err| { | |
| eprintln!("Failed to generate JSON schema for update_app_content tool: {err}"); | |
| JsonObject::default() | |
| }); | |
| rmcp::model::Tool::new( | |
| "update_app_content".to_string(), | |
| "Generate updated content for an existing Goose app. Returns the improved HTML code, updated description, and optionally updated window properties.".to_string(), | |
| schema, |
| } catch (err) { | ||
| console.error('Failed to export app:', err); | ||
| setError(err instanceof Error ? err.message : 'Failed to export app'); | ||
| } |
There was a problem hiding this comment.
The error is silently caught and logged but not displayed to the user. When an app fails to export, the user should see an error message in the UI rather than just a console log.
| export function maybeHandlePlatformEvent(notification: unknown, sessionId: string): void { | ||
| if (notification && typeof notification === 'object' && 'method' in notification) { | ||
| const msg = notification as { method?: string; params?: unknown }; | ||
| if (msg.method === 'platform_event' && msg.params) { | ||
| window.dispatchEvent( | ||
| new CustomEvent('platform-event', { | ||
| detail: { ...msg.params, sessionId }, | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The variable name 'msg' is being destructured but 'notification' was already type-checked. The redundant type assertion could be simplified by directly checking the structure or using a more specific type guard.
| export function maybeHandlePlatformEvent(notification: unknown, sessionId: string): void { | |
| if (notification && typeof notification === 'object' && 'method' in notification) { | |
| const msg = notification as { method?: string; params?: unknown }; | |
| if (msg.method === 'platform_event' && msg.params) { | |
| window.dispatchEvent( | |
| new CustomEvent('platform-event', { | |
| detail: { ...msg.params, sessionId }, | |
| }) | |
| ); | |
| } | |
| interface PlatformNotification { | |
| method?: string; | |
| params?: Record<string, unknown>; | |
| } | |
| function isPlatformNotification(notification: unknown): notification is PlatformNotification { | |
| if (!notification || typeof notification !== 'object') { | |
| return false; | |
| } | |
| const maybe = notification as { method?: unknown; params?: unknown }; | |
| if (typeof maybe.method !== 'string') { | |
| return false; | |
| } | |
| if (maybe.params === undefined || maybe.params === null || typeof maybe.params !== 'object') { | |
| return false; | |
| } | |
| return true; | |
| } | |
| export function maybeHandlePlatformEvent(notification: unknown, sessionId: string): void { | |
| if (isPlatformNotification(notification) && notification.method === 'platform_event') { | |
| window.dispatchEvent( | |
| new CustomEvent('platform-event', { | |
| detail: { ...notification.params, sessionId }, | |
| }) | |
| ); |
| let result = if let Some(head_pos) = html.find("</head>") { | ||
| let mut result = html.clone(); | ||
| result.insert_str(head_pos, &scripts); |
There was a problem hiding this comment.
The insert_str method can panic if the position is not on a UTF-8 character boundary. While HTML is typically ASCII-safe, consider using safer string manipulation methods or validating the position before insertion.
| await window.electron.launchApp(targetApp).catch((err) => { | ||
| console.error('Failed to launch newly created app:', err); | ||
| }); | ||
| } | ||
| break; | ||
|
|
||
| case 'app_updated': | ||
| if (targetApp) { | ||
| await window.electron.refreshApp(targetApp).catch((err) => { | ||
| console.error('Failed to refresh updated app:', err); | ||
| }); | ||
| } | ||
| break; | ||
|
|
||
| case 'app_deleted': | ||
| if (app_name) { | ||
| await window.electron.closeApp(app_name).catch((err) => { | ||
| console.error('Failed to close deleted app:', err); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The error messages logged here are swallowed without proper user feedback. If app launch, refresh, or close operations fail, the user won't see any notification in the UI. Consider emitting these errors through the notification system or providing user-visible feedback.
|
|
||
| const workingDir = app.getPath('home'); | ||
| const extensionName = gooseApp.mcpServer ?? ''; | ||
| const extensionName = gooseApp.mcpServers?.[0] ?? ''; |
There was a problem hiding this comment.
Using optional chaining with array index 0 could fail silently if mcpServers is an empty array. Consider checking array length or handling the case where no MCP servers are defined.
| const extensionName = gooseApp.mcpServers?.[0] ?? ''; | |
| const extensionName = | |
| Array.isArray(gooseApp.mcpServers) && gooseApp.mcpServers.length > 0 | |
| ? gooseApp.mcpServers[0] | |
| : ''; |
| } catch (err) { | ||
| console.error('Failed to import app:', err); | ||
| setError(err instanceof Error ? err.message : 'Failed to import app'); | ||
| } | ||
| event.target.value = ''; |
There was a problem hiding this comment.
The file input value is cleared after handling, but if the upload fails, the user won't be able to retry with the same file easily. Consider keeping the file selected or providing a clearer retry mechanism.
| } catch (err) { | |
| console.error('Failed to import app:', err); | |
| setError(err instanceof Error ? err.message : 'Failed to import app'); | |
| } | |
| event.target.value = ''; | |
| event.target.value = ''; | |
| } catch (err) { | |
| console.error('Failed to import app:', err); | |
| setError(err instanceof Error ? err.message : 'Failed to import app'); | |
| } |
| cache.ensure_default_apps(); | ||
| Ok(cache) | ||
| } | ||
|
|
||
| fn ensure_default_apps(&self) { | ||
| if self.get_app(APPS_EXTENSION_NAME, "apps://clock").is_none() { | ||
| if let Ok(mut clock_app) = GooseApp::from_html(CLOCK_HTML) { | ||
| clock_app.mcp_servers = vec![APPS_EXTENSION_NAME.to_string()]; | ||
| let _ = self.store_app(&clock_app); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The duplicate check in ensure_default_apps ensures the clock app is only created once in the cache. However, the same check is performed in apps_extension.rs (line 154). This duplication could lead to inconsistencies if the logic diverges. Consider consolidating this initialization logic in one place.
| cache.ensure_default_apps(); | |
| Ok(cache) | |
| } | |
| fn ensure_default_apps(&self) { | |
| if self.get_app(APPS_EXTENSION_NAME, "apps://clock").is_none() { | |
| if let Ok(mut clock_app) = GooseApp::from_html(CLOCK_HTML) { | |
| clock_app.mcp_servers = vec![APPS_EXTENSION_NAME.to_string()]; | |
| let _ = self.store_app(&clock_app); | |
| } | |
| } | |
| } | |
| Ok(cache) | |
| } |
| fn schema<T: JsonSchema>() -> JsonObject { | ||
| serde_json::to_value(schema_for!(T)) | ||
| .map(|v| v.as_object().unwrap().clone()) | ||
| .expect("valid schema") | ||
| } | ||
|
|
There was a problem hiding this comment.
This duplicate schema function exists both at line 242 and line 635. Consider removing one and using the same function throughout the module to avoid duplication.
| fn schema<T: JsonSchema>() -> JsonObject { | |
| serde_json::to_value(schema_for!(T)) | |
| .map(|v| v.as_object().unwrap().clone()) | |
| .expect("valid schema") | |
| } |
* main: docs: ml-based prompt injection detection (#6627) Strip the audience for compacting (#6646) chore(release): release version 1.21.0 (minor) (#6634) add collapsable chat nav (#6649) fix: capitalize Rust in CONTRIBUTING.md (#6640) chore(deps): bump lodash from 4.17.21 to 4.17.23 in /ui/desktop (#6623) Vibe mcp apps (#6569) Add session forking capability (#5882) chore(deps): bump lodash from 4.17.21 to 4.17.23 in /documentation (#6624) fix(docs): use named import for globby v13 (#6639) PR Code Review (#6043) fix(docs): use dynamic import for globby ESM module (#6636) chore: trigger CI Document tab completion (#6635) Install goose-mcp crate dependencies (#6632) feat(goose): standardize agent-session-id for session correlation (#6626)
Co-authored-by: Douwe Osinga <douwe@squareup.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Michael Neale <michael.neale@gmail.com> Signed-off-by: fbalicchia <fbalicchia@cuebiq.com>
* origin/main: Fix GCP Vertex AI global endpoint support for Gemini 3 models (#6187) fix: macOS keychain infinite prompt loop (#6620) chore: reduce duplicate or unused cargo deps (#6630) feat: codex subscription support (#6600) smoke test allow pass for flaky providers (#6638) feat: Add built-in skill for goose documentation reference (#6534) Native images (#6619) docs: ml-based prompt injection detection (#6627) Strip the audience for compacting (#6646) chore(release): release version 1.21.0 (minor) (#6634) add collapsable chat nav (#6649) fix: capitalize Rust in CONTRIBUTING.md (#6640) chore(deps): bump lodash from 4.17.21 to 4.17.23 in /ui/desktop (#6623) Vibe mcp apps (#6569) Add session forking capability (#5882) chore(deps): bump lodash from 4.17.21 to 4.17.23 in /documentation (#6624) fix(docs): use named import for globby v13 (#6639) PR Code Review (#6043) fix(docs): use dynamic import for globby ESM module (#6636) # Conflicts: # Cargo.lock # crates/goose-server/src/routes/session.rs
Summary
introduces a apps platform extension that lets the user vibe code "mcp" apps. still a work in progress very much, but is kinda nice:
goose_apps.mp4